Skip to content

Support for the maxlag parameter (with retries) in API requests (fixes #92)#112

Merged
waldyrious merged 5 commits intohamstar:masterfrom
Xymph:92-maxlag-param
Aug 23, 2021
Merged

Support for the maxlag parameter (with retries) in API requests (fixes #92)#112
waldyrious merged 5 commits intohamstar:masterfrom
Xymph:92-maxlag-param

Conversation

@Xymph
Copy link
Copy Markdown
Collaborator

@Xymph Xymph commented Jul 7, 2021

This parameter insures that API requests can be dropped after the specified number of seconds by a replicated wiki server cluster (such as Wikipedia) if it is strained too much. If a maxlag response is returned, Wikimate delays (sleeps) for a given number of seconds before retrying the request. It can do this indefinitely, or for a limited number of retries. See the documentation at https://www.mediawiki.org/wiki/Manual:Maxlag_parameter

For the implementation I looked around at other PHP MediaWiki libraries for inspiration, and in the bot functions table (linked from here) found an abandoned library called Pillar. Here the request class offered a useful example of handling the lag.

@waldyrious
Copy link
Copy Markdown
Collaborator

LGTM, but I'll let this one simmer for a couple days, in case @addshore wants to chime in.

@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Jul 8, 2021

Oh good, because I hadn't requested a review yet. Several more commits with the actual mechanism will follow today before the PR is ready for review. 😉

@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Jul 8, 2021

As part of centralizing API calls and their response processing in Wikimate::request(), I was considering two further changes, but this requires further thought and discussion, so that went into issue #113. The changes in this PR are extensive enough already.

@Xymph Xymph force-pushed the 92-maxlag-param branch 2 times, most recently from 30b58e0 to 9013d50 Compare July 9, 2021 12:16
@Xymph Xymph force-pushed the 92-maxlag-param branch from 9013d50 to 9553e3f Compare July 10, 2021 14:40
Copy link
Copy Markdown
Collaborator

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a bunch of mostly inconsequential suggestions inline, but the one in line 215 of Wikimate.php may actually be a bug. Please take a look.

Comment thread Wikimate.php Outdated
Comment thread Wikimate.php Outdated
Comment thread Wikimate.php Outdated
Comment thread Wikimate.php Outdated
Comment thread Wikimate.php Outdated
Comment thread Wikimate.php Outdated
Comment thread Wikimate.php
Comment thread Wikimate.php Outdated
Comment thread Wikimate.php Outdated
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Aug 16, 2021

All comment improvements and two code fixes applied locally.

Just wondering how to commit them: one bulk commit is easy, splitting all changes into separate commits and rebasing all with the previous ones would be more cumbersome, but the end result probably cleaner. Any advice?

@waldyrious
Copy link
Copy Markdown
Collaborator

Just wondering how to commit them: one bulk commit is easy, splitting all changes into separate commits and rebasing all with the previous ones would be more cumbersome, but the end result probably cleaner. Any advice?

Splitting and rebasing is preferred in ideal conditions, but given the delay my late review has caused already, I don't feel it's inadequate to apply them in a separate commit. It's not like the past git history of the project is pristine anyway :)

@Xymph

This comment has been minimized.

@waldyrious

This comment has been minimized.

@waldyrious

This comment has been minimized.

@Xymph

This comment has been minimized.

@Xymph

This comment has been minimized.

@waldyrious

This comment has been minimized.

@waldyrious

This comment has been minimized.

@Xymph

This comment has been minimized.

@waldyrious
Copy link
Copy Markdown
Collaborator

waldyrious commented Aug 23, 2021

Wow, sorry to make you go through all the drawings -- unless you aspire a new career as git-diagram artist. smiley

Haha, no worries, it was fun! In fact, don't tell anyone yet, but I've already bought styrofoam balls to try and make my own version of the "Git for Ages 4 And Up" video 😉

This looks like the correct situation we wanted from your steps too, only with different commit IDs.

So I think we're good and ready for merge?

Yep, that appears to be the (topologically) correct commit graph :) Should be ready to merge. I'll go ahead and do it to not stall this any longer, and will also collapse the git-related comments above to clean up the thread a bit.

Copy link
Copy Markdown
Collaborator

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@waldyrious waldyrious merged commit bf82f59 into hamstar:master Aug 23, 2021
@waldyrious
Copy link
Copy Markdown
Collaborator

On a separate note, just to be sure — did you get my latest emails about the rendered docs?

@Xymph Xymph deleted the 92-maxlag-param branch August 23, 2021 15:31
@Xymph
Copy link
Copy Markdown
Collaborator Author

Xymph commented Aug 23, 2021

Haha, no worries, it was fun! In fact, don't tell anyone yet, but I've already bought styrofoam balls to try and make my own version of the "Git for Ages 4 And Up" video

Don't have to tell, as you just did in a public comment there. 😉 But another contribution to understanding git should be worthwhile, so good luck with that. 👍

Yep, that appears to be the (topologically) correct commit graph :) Should be ready to merge. I'll go ahead and do it to not stall this any longer, and will also collapse the git-related comments above to clean up the thread a bit.

Thanks, phew, finally. 😅

On a separate note, just to be sure — did you get my latest emails about the rendered docs?

Yes, but back-burnered doing anything in that area for two reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants